Repository navigation
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Ready to review this PR? Stage has broken it down into 6 individual chapters for you: 
Chapters generated by Stage for commit 9283ccd on Jul 20, 2026 2:02am UTC. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe desktop host-service coordinator now performs authenticated health checks on running instances, tracks consecutive failures, and recovers wedged processes. Health polling respects its total deadline. Application startup supplies a shared configuration provider for the watchdog and development reload. ChangesHost service watchdog
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HostServiceCoordinator
participant RunningHostService
participant ConfigProvider
HostServiceCoordinator->>RunningHostService: Send authenticated health probe
RunningHostService-->>HostServiceCoordinator: Return probe result
HostServiceCoordinator->>ConfigProvider: Load SpawnConfig after threshold
ConfigProvider-->>HostServiceCoordinator: Return config or null
HostServiceCoordinator->>RunningHostService: Reset and force-kill wedged PID
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR adds a health watchdog to
Confidence Score: 4/5Safe to merge — the watchdog is additive and isolated behind its own in-flight guard; the existing exit-listener and reset/spawn paths are unchanged. The pid-staleness check, in-flight guard, and SIGTERM→SIGKILL escalation are all correctly implemented and well-tested. Two minor design points worth noting: the teardown callback from apps/desktop/src/main/lib/host-service-coordinator.ts — the failure-counter clear-before-config-check block; apps/desktop/src/main/index.ts — the discarded cleanup callback.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/main/lib/host-service-coordinator.ts | Adds health watchdog: enableHealthWatchdog/runWatchdogTick with in-flight guard, pid-aware staleness checks, SIGTERM→SIGKILL escalation, and exported threshold constants. Logic is sound; minor design note around clearing the failure counter before confirming config availability. |
| apps/desktop/src/main/index.ts | Extracts hostServiceConfigProvider lambda and calls enableHealthWatchdog unconditionally (not just dev); also reuses the lambda for enableDevReload. The teardown callback from enableHealthWatchdog is discarded. |
| apps/desktop/src/main/lib/host-service-coordinator.test.ts | Adds 6 well-targeted watchdog unit tests covering healthy clear, sub-threshold accumulation, threshold restart with SIGKILL escalation, no-config skip, non-running skip, and pid-staleness guard. Uses direct runWatchdogTick invocation instead of real timers. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Timer as setInterval (15 s)
participant Watchdog as runWatchdogTick
participant Health as pollHealthCheck
participant Coord as HostServiceCoordinator
participant OS as OS / child process
Timer->>Watchdog: tick (if not in-flight)
Watchdog->>Watchdog: "set watchdogTickInFlight = true"
loop for each running instance
Watchdog->>Health: GET /trpc/health.check (3 s timeout)
Health-->>Watchdog: true → clear failures, continue
Health-->>Watchdog: false → increment watchdogFailures[org]
alt "failures < THRESHOLD (3)"
Watchdog->>Watchdog: continue (log warning)
else "failures == THRESHOLD"
Watchdog->>Watchdog: delete watchdogFailures[org]
Watchdog->>Coord: "configProvider() → SpawnConfig | null"
alt config available
Watchdog->>Coord: reset(org, config)
Coord->>OS: stop() → SIGTERM tracked pid
Coord->>OS: SIGKILL manifest pid (if alive)
Coord->>Coord: spawn new child
Watchdog->>OS: SIGKILL wedgedPid (if still alive)
else config null (logged out)
Watchdog->>Watchdog: skip restart
end
end
end
Watchdog->>Watchdog: "set watchdogTickInFlight = false"
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Timer as setInterval (15 s)
participant Watchdog as runWatchdogTick
participant Health as pollHealthCheck
participant Coord as HostServiceCoordinator
participant OS as OS / child process
Timer->>Watchdog: tick (if not in-flight)
Watchdog->>Watchdog: "set watchdogTickInFlight = true"
loop for each running instance
Watchdog->>Health: GET /trpc/health.check (3 s timeout)
Health-->>Watchdog: true → clear failures, continue
Health-->>Watchdog: false → increment watchdogFailures[org]
alt "failures < THRESHOLD (3)"
Watchdog->>Watchdog: continue (log warning)
else "failures == THRESHOLD"
Watchdog->>Watchdog: delete watchdogFailures[org]
Watchdog->>Coord: "configProvider() → SpawnConfig | null"
alt config available
Watchdog->>Coord: reset(org, config)
Coord->>OS: stop() → SIGTERM tracked pid
Coord->>OS: SIGKILL manifest pid (if alive)
Coord->>Coord: spawn new child
Watchdog->>OS: SIGKILL wedgedPid (if still alive)
else config null (logged out)
Watchdog->>Watchdog: skip restart
end
end
end
Watchdog->>Watchdog: "set watchdogTickInFlight = false"
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/desktop/src/main/index.ts:422
**Cleanup function from `enableHealthWatchdog` is discarded**
`enableHealthWatchdog` returns a teardown callback that clears the `setInterval` timer and the `watchdogFailures` map. Discarding it means there is no programmatic way to stop the watchdog once started — the only protection against the timer outliving its intended scope is `timer.unref()`. This is technically safe for the current Electron singleton (the process exits on quit), but it departs from the pattern established by `enableDevReload`, and would cause an orphaned, uncancellable timer if the coordinator is ever reset or recreated in tests or future code paths.
### Issue 2 of 2
apps/desktop/src/main/lib/host-service-coordinator.ts:436-438
**Failure counter cleared before config availability check**
The `watchdogFailures` counter is deleted unconditionally at threshold before `configProvider()` is even called (line 436). If `configProvider()` returns `null` (user logged out), the counter stays deleted and the next restart attempt requires `HEALTH_WATCHDOG_FAILURE_THRESHOLD` additional consecutive failures — 45 more seconds of a wedged service before recovery is attempted again. Deleting the counter only after a successful restart call would keep the watchdog "armed" and retry on the very next tick instead of resetting the clock.
Reviews (1): Last reviewed commit: "fix(desktop): auto-recover unresponsive ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| const hostServiceConfigProvider = async () => { | ||
| const { token } = await loadToken(); | ||
| if (!token) return null; |
There was a problem hiding this comment.
enableHealthWatchdog is discarded
enableHealthWatchdog returns a teardown callback that clears the setInterval timer and the watchdogFailures map. Discarding it means there is no programmatic way to stop the watchdog once started — the only protection against the timer outliving its intended scope is timer.unref(). This is technically safe for the current Electron singleton (the process exits on quit), but it departs from the pattern established by enableDevReload, and would cause an orphaned, uncancellable timer if the coordinator is ever reset or recreated in tests or future code paths.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/index.ts
Line: 422
Comment:
**Cleanup function from `enableHealthWatchdog` is discarded**
`enableHealthWatchdog` returns a teardown callback that clears the `setInterval` timer and the `watchdogFailures` map. Discarding it means there is no programmatic way to stop the watchdog once started — the only protection against the timer outliving its intended scope is `timer.unref()`. This is technically safe for the current Electron singleton (the process exits on quit), but it departs from the pattern established by `enableDevReload`, and would cause an orphaned, uncancellable timer if the coordinator is ever reset or recreated in tests or future code paths.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Leaving as-is intentionally: this mirrors the existing enableDevReload call site, which also discards its disposer. The coordinator is a process-lifetime singleton and the timer is unref()d, so there's no scope it can outlive today. Happy to wire both disposers into a teardown if a maintainer prefers.
| this.watchdogFailures.delete(organizationId); | ||
| const config = await configProvider(); | ||
| if (!config) continue; |
There was a problem hiding this comment.
The watchdogFailures counter is deleted unconditionally at threshold before configProvider() is even called (line 436). If configProvider() returns null (user logged out), the counter stays deleted and the next restart attempt requires HEALTH_WATCHDOG_FAILURE_THRESHOLD additional consecutive failures — 45 more seconds of a wedged service before recovery is attempted again. Deleting the counter only after a successful restart call would keep the watchdog "armed" and retry on the very next tick instead of resetting the clock.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/lib/host-service-coordinator.ts
Line: 436-438
Comment:
**Failure counter cleared before config availability check**
The `watchdogFailures` counter is deleted unconditionally at threshold before `configProvider()` is even called (line 436). If `configProvider()` returns `null` (user logged out), the counter stays deleted and the next restart attempt requires `HEALTH_WATCHDOG_FAILURE_THRESHOLD` additional consecutive failures — 45 more seconds of a wedged service before recovery is attempted again. Deleting the counter only after a successful restart call would keep the watchdog "armed" and retry on the very next tick instead of resetting the clock.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in 4666951 — the counter now survives a null config and is only cleared once recovery actually runs, so the watchdog retries on the next tick instead of waiting out another full threshold.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Architecture diagram
sequenceDiagram
participant Main as Main Process Startup
participant Coordinator as HostServiceCoordinator
participant Child as HostService Child Process
participant Renderer as Renderer (Client)
participant OS as Operating System
Note over Main,Coordinator: Watchdog Initialization (at startup)
Main->>Coordinator: enableHealthWatchdog(configProvider)
activate Coordinator
Coordinator->>Coordinator: Start interval timer (HEALTH_WATCHDOG_INTERVAL_MS)
deactivate Coordinator
loop Every 15 seconds
Note over Coordinator: runWatchdogTick()
Coordinator->>Coordinator: For each "running" instance
alt Instance not "running" or has pending start
Coordinator->>Coordinator: Clear failure counter, skip
else Instance is "running"
Coordinator->>Child: pollHealthCheck(/trpc/health.check, timeout 3s)
alt Healthy response
Coordinator->>Coordinator: Clear failure counter
else Timeout / Unhealthy response
Coordinator->>Coordinator: Increment failure count
alt Failure count < HEALTH_WATCHDOG_FAILURE_THRESHOLD (3)
Note over Coordinator: Log warning, no action
else Failure count >= threshold
Coordinator->>Coordinator: Discard failure counter
Coordinator->>Coordinator: Get spawn config via provider
alt No spawn config (e.g., logged out)
Note over Coordinator: Skip restart
else Config available
Coordinator->>Child: stop() – send SIGTERM
alt Child process still alive after stop()
Coordinator->>OS: SIGKILL (escalation)
end
Coordinator->>Coordinator: reset() – spawn new child
Coordinator-->>Child: New host‑service instance
Note over Coordinator,Child: New port/secret registered
end
end
end
end
alt Child replaced mid‑probe (PID changed)
Note over Coordinator: Discard probe result, no failure counted
end
end
Note over Renderer,Coordinator: Existing connection flow (unchanged)
Renderer->>Coordinator: getConnection(organizationId)
alt Success
Coordinator-->>Renderer: Port and secret (new or current)
Renderer->>Child: createSession(...)
Child-->>Renderer: Session handle
else Still waiting for child
Coordinator-->>Renderer: Poll / status events (5s interval)
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/host-service-coordinator.test.ts (1)
247-275: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
HostServiceStatusinstead ofstringin the test seam.The weakened type permits impossible production states and can hide contract drift. Reuse the exported status type for both declarations.
As per coding guidelines, “Avoid
anyunless it is necessary; preserve type safety.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/lib/host-service-coordinator.test.ts` around lines 247 - 275, The WatchdogInternals test seam and seedInstance overrides use string for service status, weakening the contract. Import and reuse the exported HostServiceStatus type for both status declarations while leaving the remaining fields and behavior unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/lib/host-service-coordinator.ts`:
- Around line 458-476: Update the watchdog recovery flow around reset() and
wedgedPid so the fallback PID is passed into reset and force-killed immediately
after stop() completes, before spawning or awaiting the replacement and health
polling. Remove the delayed isProcessAlive/killProcess escalation after reset
returns, while preserving the existing watchdog failure handling and replacement
reset behavior.
- Around line 405-409: Update the health-probe flow around pollHealthCheck to
enforce HEALTH_WATCHDOG_PROBE_TIMEOUT_MS as a hard deadline. Track elapsed time
or a deadline, pass only the remaining duration to each fetch, and cap any retry
sleep to that remaining time so the function returns within three seconds even
when probes run serially.
- Around line 103-105: Update watchdogFailures and its handling in the watchdog,
stop, exit, and spawn paths to bind each failure count to the specific child
instance rather than only its organization. Store the child identity alongside
the count and reset or replace the entry whenever a new child is spawned,
stopped, or exits, ensuring a replacement child’s first failed probe starts at
zero.
- Around line 459-468: Update the watchdog flow around reset() and its catch
block to track failed recoveries separately from active instances, so a respawn
failure remains eligible for later attempts even when the instance is absent or
non-running. Retry pending recoveries on subsequent ticks using bounded backoff,
while preserving the existing threshold behavior and clearing the pending state
after successful recovery.
---
Nitpick comments:
In `@apps/desktop/src/main/lib/host-service-coordinator.test.ts`:
- Around line 247-275: The WatchdogInternals test seam and seedInstance
overrides use string for service status, weakening the contract. Import and
reuse the exported HostServiceStatus type for both status declarations while
leaving the remaining fields and behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60076a7c-157f-4e67-af86-8404d492d899
📒 Files selected for processing (3)
apps/desktop/src/main/index.tsapps/desktop/src/main/lib/host-service-coordinator.test.tsapps/desktop/src/main/lib/host-service-coordinator.ts
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/lib/host-service-coordinator.ts (1)
399-403: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winInvalidate in-flight ticks during teardown.
clearInterval()does not cancel a tick already awaiting a probe or configuration. That tick can repopulate the cleared map or reset a service after teardown. Use an enable-generation/cancellation token checked after asynchronous boundaries, and only clearwatchdogTimerwhen it still equalstimer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/lib/host-service-coordinator.ts` around lines 399 - 403, Update the watchdog teardown callback in the host-service coordinator to invalidate in-flight ticks using an enable-generation or cancellation token, checking it after each asynchronous probe or configuration boundary before mutating state or resetting services. Only set watchdogTimer to null when it still references the captured timer, while preserving cleanup of watchdogFailures during teardown.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/lib/host-service-coordinator.ts`:
- Around line 521-527: Revalidate the recovery state after configProvider()
resolves and before calling startWithPreferredPorts() in the watchdog retry
flow, returning or continuing when stop(organizationId) has removed the recovery
entry. Add a deferred-configuration test that invokes stop() while the provider
is pending, then resolves it and verifies the service is not respawned.
---
Outside diff comments:
In `@apps/desktop/src/main/lib/host-service-coordinator.ts`:
- Around line 399-403: Update the watchdog teardown callback in the host-service
coordinator to invalidate in-flight ticks using an enable-generation or
cancellation token, checking it after each asynchronous probe or configuration
boundary before mutating state or resetting services. Only set watchdogTimer to
null when it still references the captured timer, while preserving cleanup of
watchdogFailures during teardown.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 680cdac5-890e-433b-98d6-39f35cb04364
📒 Files selected for processing (3)
apps/desktop/src/main/lib/host-service-coordinator.test.tsapps/desktop/src/main/lib/host-service-coordinator.tsapps/desktop/src/main/lib/host-service-utils.ts
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/main/lib/host-service-coordinator.ts">
<violation number="1" location="apps/desktop/src/main/lib/host-service-coordinator.ts:238">
P0: A normally exiting child can have its PID recycled after `stop()` sends SIGTERM, causing this watchdog escalation to SIGKILL an unrelated local process. Bind escalation to process identity or avoid the post-SIGTERM PID-only kill path; `isProcessAlive` verifies only that some process owns the PID.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
cc93ea8 to
9283ccd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/lib/host-service-coordinator.ts`:
- Around line 443-591: Update loadToken in auth-functions.ts to inspect the
decrypted token’s expiresAt before returning it. Return null when the expiration
is in the past, while preserving the existing token return behavior for valid or
non-expiring tokens so watchdog recovery waits for reauthentication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 910ed8df-f44c-4223-96c1-e7a5b27f1ceb
📒 Files selected for processing (4)
apps/desktop/src/main/index.tsapps/desktop/src/main/lib/host-service-coordinator.test.tsapps/desktop/src/main/lib/host-service-coordinator.tsapps/desktop/src/main/lib/host-service-utils.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/main/lib/host-service-utils.ts
- apps/desktop/src/main/index.ts
- apps/desktop/src/main/lib/host-service-coordinator.test.ts
| private async runWatchdogTick( | ||
| configProvider: () => Promise<SpawnConfig | null>, | ||
| ): Promise<void> { | ||
| // A wedged instance makes each probe wait out its full timeout — skip | ||
| // the tick instead of stacking probes against the same instance. | ||
| if (this.watchdogTickInFlight) return; | ||
| this.watchdogTickInFlight = true; | ||
|
|
||
| try { | ||
| for (const [organizationId, instance] of [...this.instances]) { | ||
| if ( | ||
| instance.status !== "running" || | ||
| this.pendingStarts.has(organizationId) | ||
| ) { | ||
| continue; | ||
| } | ||
|
|
||
| const healthy = await pollHealthCheck( | ||
| `http://127.0.0.1:${instance.port}`, | ||
| instance.secret, | ||
| HEALTH_WATCHDOG_PROBE_TIMEOUT_MS, | ||
| ); | ||
|
|
||
| // The instance may have been stopped or replaced while the probe | ||
| // was in flight — a stale probe result must not count against the | ||
| // replacement child. | ||
| const current = this.instances.get(organizationId); | ||
| if ( | ||
| !current || | ||
| current.pid !== instance.pid || | ||
| current.status !== "running" | ||
| ) { | ||
| continue; | ||
| } | ||
|
|
||
| if (healthy) { | ||
| this.watchdogFailures.delete(organizationId); | ||
| continue; | ||
| } | ||
|
|
||
| // Bind the count to this child's pid: a freshly-spawned child | ||
| // starts from 1 instead of inheriting its predecessor's failures. | ||
| const prev = this.watchdogFailures.get(organizationId); | ||
| const failureCount = prev?.pid === instance.pid ? prev.count + 1 : 1; | ||
| this.watchdogFailures.set(organizationId, { | ||
| pid: instance.pid, | ||
| count: failureCount, | ||
| }); | ||
| log.warn( | ||
| `[host-service:${organizationId}] health check failed (${failureCount}/${HEALTH_WATCHDOG_FAILURE_THRESHOLD}) on port ${instance.port}`, | ||
| ); | ||
| if (failureCount < HEALTH_WATCHDOG_FAILURE_THRESHOLD) continue; | ||
|
|
||
| // Keep the counter until recovery actually runs — if no config is | ||
| // available (logged out) the watchdog stays armed and retries on | ||
| // the next tick instead of waiting out a full threshold again. | ||
| const config = await configProvider(); | ||
| if (!config) continue; | ||
|
|
||
| // Revalidate after the await — a manual restart may have replaced | ||
| // the child while the config was being fetched. | ||
| const latest = this.instances.get(organizationId); | ||
| if ( | ||
| !latest || | ||
| latest.pid !== instance.pid || | ||
| latest.status !== "running" || | ||
| this.pendingStarts.has(organizationId) | ||
| ) { | ||
| this.watchdogFailures.delete(organizationId); | ||
| continue; | ||
| } | ||
|
|
||
| log.error( | ||
| `[host-service:${organizationId}] unresponsive for ${HEALTH_WATCHDOG_FAILURE_THRESHOLD} consecutive health checks — force-restarting`, | ||
| ); | ||
| try { | ||
| // alsoKillPid escalates the wedged child to SIGKILL inside | ||
| // reset(), after stop() but before the slow respawn — waiting | ||
| // until reset() returned would leave a window where the pid is | ||
| // recycled and the SIGKILL hits an innocent process. | ||
| await this.reset(organizationId, config, { | ||
| alsoKillPid: instance.pid, | ||
| }); | ||
| this.watchdogFailures.delete(organizationId); | ||
| } catch (error) { | ||
| log.error( | ||
| `[host-service:${organizationId}] watchdog restart failed:`, | ||
| error, | ||
| ); | ||
| // reset()'s stop() disarmed the entry; re-arm it so the | ||
| // no-instance retry pass below keeps trying on later ticks. | ||
| this.watchdogFailures.set(organizationId, { | ||
| pid: instance.pid, | ||
| count: failureCount, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // Orgs whose recovery failed have no tracked instance and are | ||
| // invisible to the loop above — keep retrying those each tick until | ||
| // a spawn sticks. Deliberate stops never land here: stop() disarms | ||
| // the entry. | ||
| for (const [organizationId, entry] of [...this.watchdogFailures]) { | ||
| if ( | ||
| this.instances.has(organizationId) || | ||
| this.pendingStarts.has(organizationId) | ||
| ) { | ||
| continue; | ||
| } | ||
| if (entry.count < HEALTH_WATCHDOG_FAILURE_THRESHOLD) { | ||
| // Stale sub-threshold entry (e.g. the child crashed on its | ||
| // own after a blip) — nothing to recover, drop it. | ||
| this.watchdogFailures.delete(organizationId); | ||
| continue; | ||
| } | ||
| const config = await configProvider(); | ||
| if (!config) continue; | ||
| // Revalidate after the await — a concurrent stop() disarms the | ||
| // entry (deliberate shutdown) and a concurrent start() makes the | ||
| // respawn redundant; neither must be overridden here. | ||
| if ( | ||
| this.watchdogFailures.get(organizationId) !== entry || | ||
| this.instances.has(organizationId) || | ||
| this.pendingStarts.has(organizationId) | ||
| ) { | ||
| continue; | ||
| } | ||
| log.warn( | ||
| `[host-service:${organizationId}] retrying respawn after failed watchdog recovery`, | ||
| ); | ||
| try { | ||
| await this.startWithPreferredPorts(organizationId, config); | ||
| this.watchdogFailures.delete(organizationId); | ||
| } catch (error) { | ||
| log.error( | ||
| `[host-service:${organizationId}] watchdog respawn retry failed:`, | ||
| error, | ||
| ); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| // The tick runs detached from a timer — a rejection here (e.g. the | ||
| // config lookup throwing) would otherwise surface as an unhandled | ||
| // rejection. Log and let the next tick retry. | ||
| log.error("[host-service] watchdog tick failed:", error); | ||
| } finally { | ||
| this.watchdogTickInFlight = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C4 'enableHealthWatchdog\s*\(' apps/desktop/src/main/index.tsRepository: superset-sh/superset
Length of output: 516
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== index.ts occurrences ==\n'
rg -n -C 3 'loadToken|enableHealthWatchdog|authToken|NEXT_PUBLIC_API_URL' apps/desktop/src/main/index.ts
printf '\n== files defining loadToken ==\n'
rg -n --files | rg 'loadToken|auth.*token|token' | sed -n '1,120p'
printf '\n== likely auth/token files ==\n'
fd -a -t f '.*(token|auth).*' apps/desktop/src/main apps/desktop/src -E '*.map' | sed -n '1,160p'Repository: superset-sh/superset
Length of output: 3115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== auth-functions.ts outline ==\n'
ast-grep outline apps/desktop/src/lib/trpc/routers/auth/utils/auth-functions.ts --view expanded
printf '\n== auth-functions.ts relevant section ==\n'
nl -ba apps/desktop/src/lib/trpc/routers/auth/utils/auth-functions.ts | sed -n '1,260p'
printf '\n== token storage/read helpers ==\n'
rg -n -C 3 'function loadToken|const loadToken|export .*loadToken|authToken|refresh|expires|token' apps/desktop/src/lib/trpc/routers/auth/utils/auth-functions.ts apps/desktop/src/renderer/lib/auth-client.ts apps/desktop/src/renderer/lib/host-service-auth.ts apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsxRepository: superset-sh/superset
Length of output: 768
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== auth-functions.ts outline ==\n'
ast-grep outline apps/desktop/src/lib/trpc/routers/auth/utils/auth-functions.ts --view expanded
printf '\n== auth-functions.ts relevant section ==\n'
nl -ba apps/desktop/src/lib/trpc/routers/auth/utils/auth-functions.ts | sed -n '1,260p'
printf '\n== token-related helpers ==\n'
rg -n -C 3 'loadToken|saveToken|clearToken|token|expires|expiry|refresh' \
apps/desktop/src/lib/trpc/routers/auth/utils/auth-functions.ts \
apps/desktop/src/renderer/lib/auth-client.ts \
apps/desktop/src/renderer/lib/host-service-auth.ts \
apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsxRepository: superset-sh/superset
Length of output: 768
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a -t f 'auth-functions.ts' apps/desktop/src | sed -n '1,20p'Repository: superset-sh/superset
Length of output: 240
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba apps/desktop/src/lib/trpc/routers/auth/utils/auth-functions.ts | sed -n '1,260p'Repository: superset-sh/superset
Length of output: 198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba apps/desktop/src/lib/trpc/routers/auth/utils/auth-functions.ts | sed -n '1,220p'Repository: superset-sh/superset
Length of output: 198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("apps/desktop/src/lib/trpc/routers/auth/utils/auth-functions.ts")
for i, line in enumerate(path.read_text().splitlines(), 1):
if 1 <= i <= 180:
print(f"{i:4d}: {line}")
PYRepository: superset-sh/superset
Length of output: 3376
Guard loadToken() against expired auth tokens. In apps/desktop/src/lib/trpc/routers/auth/utils/auth-functions.ts, loadToken() just decrypts and վերադարձs the stored token; it never checks expiresAt. If an expired token is still on disk, watchdog recovery will keep retrying with a stale authToken instead of returning null and waiting for reauth.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import * as childProcess from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/main/lib/host-service-coordinator.ts` around lines 443 -
591, Update loadToken in auth-functions.ts to inspect the decrypted token’s
expiresAt before returning it. Return null when the expiration is in the past,
while preserving the existing token return behavior for valid or non-expiring
tokens so watchdog recovery waits for reauthentication.
…tchdog A wedged-but-alive host-service child stays "running" forever — the exit listener only fires when the child dies, so today the only recovery is a manual tray restart or a reboot (ParadevAI#5699). Add a periodic health watchdog that probes every running instance and force-restarts one that misses HEALTH_WATCHDOG_FAILURE_THRESHOLD consecutive checks: - pid-bound failure counters (a fresh child starts from 1, never inherits its predecessor's failures); stale probe results can't count against a replacement child. - SIGKILL escalation happens inside reset() after stop() but before the slow respawn, so a recycled pid can't be hit by a late kill. - counter stays armed when config is unavailable (logged out) and a no-instance retry pass respawns orgs whose recovery failed, revalidating the armed entry after each config await so a concurrent stop()/start() is never overridden. - spawn() removes its "starting" entry on any startup failure so a stale ghost never looks occupied to status readers or the retry pass; the tick catches its own rejections instead of escaping the timer callback. - the watchdog only supervises children this instance owns — adopted entries (single-flight ParadevAI#5791) belong to another live instance and must never be force-killed here. - pollHealthCheck clamps both its fetch abort and retry sleep to the remaining budget so a serial probe never overruns its advertised timeout.
9283ccd to
a1d032a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/lib/host-service-coordinator.ts`:
- Around line 838-857: Guard the success path after pollHealthCheck with the
same current-instance check used by the failure cleanup: verify
this.instances.get(organizationId) === instance before setting instance.status
to "running", emitting the running status, or returning success. If the instance
was removed or replaced by stop(), avoid resurrecting it and follow the existing
startup cleanup/error handling path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f692932-ae35-477a-8325-5e5303b48100
📒 Files selected for processing (4)
apps/desktop/src/main/index.tsapps/desktop/src/main/lib/host-service-coordinator.test.tsapps/desktop/src/main/lib/host-service-coordinator.tsapps/desktop/src/main/lib/host-service-utils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/main/index.ts
- apps/desktop/src/main/lib/host-service-coordinator.test.ts
| const healthy = await pollHealthCheck( | ||
| endpoint, | ||
| secret, | ||
| HEALTH_POLL_TIMEOUT_MS, | ||
| () => childExited, | ||
| ); | ||
| } | ||
| if (!healthy) { | ||
| if (!childExited) child.kill("SIGTERM"); | ||
| throw new Error( | ||
| childExited | ||
| ? "Host service process exited during startup" | ||
| : `Host service failed to start within ${HEALTH_POLL_TIMEOUT_MS}ms`, | ||
| ); | ||
| } | ||
|
|
||
| instance.status = "running"; | ||
| instance.status = "running"; | ||
|
|
||
| log.info(`[host-service:${organizationId}] listening on port ${port}`); | ||
| this.emitStatus(organizationId, "running", "starting"); | ||
| return { port, secret, machineId: this.machineId }; | ||
| log.info(`[host-service:${organizationId}] listening on port ${port}`); | ||
| this.emitStatus(organizationId, "running", "starting"); | ||
| return { port, secret, machineId: this.machineId }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Success path can resurrect a phantom "running" status for an instance stop() already tore down.
The failure branch (Lines 858-869) now guards cleanup with this.instances.get(organizationId) === instance, but the success branch has no equivalent check. If stop(organizationId) runs while this spawn() call is awaiting pollHealthCheck (stop() isn't gated by pendingStarts, so it can act on an in-flight "starting" instance), it deletes the map entry and SIGTERMs the child — but a slow-to-die child can still let an in-flight probe return healthy=true. This code then unconditionally sets instance.status = "running" and emits a "running" event for an org the coordinator no longer tracks (and whose process was just killed), misleading any listener (renderer/tray) into believing the service is up.
🩹 Proposed fix
+ if (this.instances.get(organizationId) !== instance) {
+ // stop()/reset() replaced or removed this entry while we were
+ // waiting on health — don't resurrect a "running" event for a
+ // child the caller already tore down.
+ if (!childExited) child.kill("SIGTERM");
+ throw new Error(
+ "Host service instance was stopped during startup",
+ );
+ }
+
instance.status = "running";
-
log.info(`[host-service:${organizationId}] listening on port ${port}`);
this.emitStatus(organizationId, "running", "starting");
return { port, secret, machineId: this.machineId };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const healthy = await pollHealthCheck( | |
| endpoint, | |
| secret, | |
| HEALTH_POLL_TIMEOUT_MS, | |
| () => childExited, | |
| ); | |
| } | |
| if (!healthy) { | |
| if (!childExited) child.kill("SIGTERM"); | |
| throw new Error( | |
| childExited | |
| ? "Host service process exited during startup" | |
| : `Host service failed to start within ${HEALTH_POLL_TIMEOUT_MS}ms`, | |
| ); | |
| } | |
| instance.status = "running"; | |
| instance.status = "running"; | |
| log.info(`[host-service:${organizationId}] listening on port ${port}`); | |
| this.emitStatus(organizationId, "running", "starting"); | |
| return { port, secret, machineId: this.machineId }; | |
| log.info(`[host-service:${organizationId}] listening on port ${port}`); | |
| this.emitStatus(organizationId, "running", "starting"); | |
| return { port, secret, machineId: this.machineId }; | |
| const healthy = await pollHealthCheck( | |
| endpoint, | |
| secret, | |
| HEALTH_POLL_TIMEOUT_MS, | |
| () => childExited, | |
| ); | |
| if (!healthy) { | |
| if (!childExited) child.kill("SIGTERM"); | |
| throw new Error( | |
| childExited | |
| ? "Host service process exited during startup" | |
| : `Host service failed to start within ${HEALTH_POLL_TIMEOUT_MS}ms`, | |
| ); | |
| } | |
| if (this.instances.get(organizationId) !== instance) { | |
| // stop()/reset() replaced or removed this entry while we were | |
| // waiting on health — don't resurrect a "running" event for a | |
| // child the caller already tore down. | |
| if (!childExited) child.kill("SIGTERM"); | |
| throw new Error("Host service instance was stopped during startup"); | |
| } | |
| instance.status = "running"; | |
| log.info(`[host-service:${organizationId}] listening on port ${port}`); | |
| this.emitStatus(organizationId, "running", "starting"); | |
| return { port, secret, machineId: this.machineId }; |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import * as childProcess from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/main/lib/host-service-coordinator.ts` around lines 838 -
857, Guard the success path after pollHealthCheck with the same current-instance
check used by the failure cleanup: verify this.instances.get(organizationId) ===
instance before setting instance.status to "running", emitting the running
status, or returning success. If the instance was removed or replaced by stop(),
avoid resurrecting it and follow the existing startup cleanup/error handling
path.
What & why
Related to #5699 — "Failed to run preset / Failed to fetch (127.0.0.1:48135)". (The primary root cause — a node-pty fd leak that exhausts the system-wide pty cap — is fixed in #5714; this PR fixes the secondary failure mode where a wedged host-service is never detected.)
The
HostServiceCoordinatoronly listens for the child process exit event. A host-service that wedges without dying (blocked event loop, fd exhaustion after extended usage) therefore stays"running"forever:getConnectionkeeps handing the renderer the same port, everyterminal.createSessionfetch fails withFailed to fetch (127.0.0.1:<port>), and the only recovery is the manual tray restart — or rebooting the machine, as the reporter did. (The port in the report, 48135, is in the coordinator's stable-port range 48000–48999, confirming the renderer was talking to a local host-service the coordinator still considered healthy.)This adds a health watchdog to the coordinator:
"running"instance's/trpc/health.checkevery 15s (3s probe timeout), skipping instances with a pending start and discarding probe results if the instance was stopped/replaced mid-probe.reset()recovery path, escalating to SIGKILL on the wedged pid (a wedged process can ignorestop()'s SIGTERM).createSessionalready implements; renderer picks up the fresh connection through the existing 5sgetConnectionpoll + status events.Related: #5700 retries transient network blips on the create path; this PR addresses the persistent-until-reboot failure mode by making the coordinator detect and heal a wedged child.
How I tested it
host-service-coordinator.test.tscovering: healthy probe clears the failure counter; no restart below threshold; force-restart at threshold with SIGTERM→SIGKILL escalation; skip when no spawn config (logged-out); non-running instances not probed; probe failures don't count against a replaced child.bun test apps/desktop/src/main/lib/host-service-coordinator.test.ts— 12 pass (6 existing + 6 new).bun run lintandtsc --noEmit(apps/desktop) pass.Checklist
type(scope): subject)bun run lintandbun run typecheckpass (CI fails on lint warnings too)Summary by cubic
Adds a health watchdog to the desktop
HostServiceCoordinatorto auto-recover unresponsive host-service processes. It detects wedged, still-running children and restarts them, removing persistent “Failed to fetch (127.0.0.1:PORT)” errors without manual restarts./trpc/health.checkevery 15s (3s timeout); clamp fetch/sleep to the remaining budget; avoid overlapping ticks.stop().reset()and SIGKILL the wedged pid before respawn; skip adopted (non-owned) instances.stop()/start.enableHealthWatchdoginmainwith a shared auth/config provider; tests cover thresholds, pid-binding, retries, escalation ordering, revalidation, adopted-skip, and startup failure cleanup.Written for commit a1d032a. Summary will update on new commits.
Summary by CodeRabbit